Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theming initial proposal #4579

Merged
merged 11 commits into from
Jan 21, 2023
Merged

Theming initial proposal #4579

merged 11 commits into from
Jan 21, 2023

Conversation

prachi00
Copy link
Member

@prachi00 prachi00 commented Dec 28, 2022

Thank you for your contribution to the KodaDot NFT gallery.

👇 _ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </bsx/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Had issue bounty label?

  • Fill up your KSM address:
    Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

@netlify
Copy link

netlify bot commented Dec 28, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 6b18442
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/63cc5b9cdb0c87000baa19e2
😎 Deploy Preview https://deploy-preview-4579--koda-nuxt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@prachi00
Copy link
Member Author

prachi00 commented Dec 28, 2022

@roiLeo @yangwao @vikiival @Jarsen136 @petersopko @preschian
I am really tired of writing double css, once in light mode and then in dark mode, I researched and figured this would be the best way to make sure that css is only written once for all themes
With this approach, we can get rid of _Dark.scss and _light.scss files

Let me know if this seems okay to you, or you would prefer some other way of doing it?
I couldnt really find anything for using scss variables theming directly without any mixins

I have some time because of the holidays going on, I can do this transition, I feel like it would speed up our development by doing this once and for all

@Jarsen136
Copy link
Contributor

I really agree to find some smart way to make the theme colour switch more smoothly.
IMO, we could implement the theme switch by using CSS variables.
(ref: https://stackoverflow.com/questions/64390664/how-to-implement-switchable-themes-in-scss)

For example, in the nft-gallery project. We could define different colors for one variable on light.css and dark.css files.
On the component side, the variable could be used as var(xxxx) to transform to the real color value.

styles/themes/_light.scss

html.light-mode {
 --background-color: white;
 --title-color: #333;
}

styles/themes/_dark.scss

html.dark-mode {
 --background-color: black;
 --title-color: #fff;

}

component css file

.card {
  color: var(--title-color);
  background: var(--background-color);
}

@prachi00
Copy link
Member Author

I really agree to find some smart way to make the theme colour switch more smoothly. IMO, we could implement the theme switch by using CSS variables. (ref: https://stackoverflow.com/questions/64390664/how-to-implement-switchable-themes-in-scss)

For example, in the nft-gallery project. We could define different colors for one variable on light.css and dark.css files. On the component side, the variable could be used as var(xxxx) to transform to the real color value.

styles/themes/_light.scss

html.light-mode {
 --background-color: white;
 --title-color: #333;
}

styles/themes/_dark.scss

html.dark-mode {
 --background-color: black;
 --title-color: #fff;

}

component css file

.card {
  color: var(--title-color);
  background: var(--background-color);
}

yeah this is the most common approach I found as well

@yangwao
Copy link
Member

yangwao commented Jan 1, 2023

best way to make sure that css is only written once for all themes
With this approach, we can get rid of _Dark.scss and _light.scss files

@prachi00 hey, I really like your initiative and I am pretty sure we can reward this one!
Can you make this as an issue so we keep discussing there if anyone would have objections or suggestions?:)

This PR can still be ongoing.

@roiLeo
Copy link
Contributor

roiLeo commented Jan 2, 2023

Sure, this look good.
Will it be possible to manage different theme color according to chain selection? ref #153

@prachi00
Copy link
Member Author

prachi00 commented Jan 2, 2023

Sure, this look good. Will it be possible to manage different theme color according to chain selection? ref #153

yeah I guess we can do that, we just have to define a different object in theme.scss once the migration is done

@prachi00
Copy link
Member Author

prachi00 commented Jan 5, 2023

@exezbcz can you provide a list of colours and their dark-mode alternative colour? that would help a lot
for example k-accent for light is ff7ac3 , what is k-accent for dark ?

@prachi00
Copy link
Member Author

prachi00 commented Jan 8, 2023

@exezbcz can you provide a list of colours and their dark-mode alternative colour? that would help a lot for example k-accent for light is ff7ac3 , what is k-accent for dark ?

@exezbcz can you please tell me the colors?

@roiLeo
Copy link
Contributor

roiLeo commented Jan 9, 2023

@exezbcz can you provide a list of colours and their dark-mode alternative colour? that would help a lot for example k-accent for light is ff7ac3 , what is k-accent for dark ?

@exezbcz can you please tell me the colors?

Have you search in figma files?
https://www.figma.com/file/3iOjW12ERFRDSVnpLuhQmf/landing-page-handoff?node-id=1305%3A5612&t=2K9nVztP00JaYV8B-0

@exezbcz
Copy link
Member

exezbcz commented Jan 9, 2023

@exezbcz can you provide a list of colours and their dark-mode alternative colour? that would help a lot for example k-accent for light is ff7ac3 , what is k-accent for dark ?

@exezbcz can you please tell me the colors?

I'm sorry for the delay. To make the colors more understandable, I will add more details - maybe in form of short description. You might have a look at landing page handoff ( first page) in the meantime.

@prachi00
Copy link
Member Author

prachi00 commented Jan 10, 2023

@exezbcz can you provide a list of colours and their dark-mode alternative colour? that would help a lot for example k-accent for light is ff7ac3 , what is k-accent for dark ?

@exezbcz can you please tell me the colors?

I'm sorry for the delay. To make the colors more understandable, I will add more details - maybe in form of short description. You might have a look at landing page handoff ( first page) in the meantime.

okay, from figma it isnt that clear, as not all the colors seem to have a dark mode equivalent (for example whats dark mode equivalent for k-accentlight or for k-hovergrey), it would be helpful if we could keep the variable name same and then have two different colors for it, one for light and one for dark.
@exezbcz

@prachi00
Copy link
Member Author

@roiLeo also, since we no use oruga, will we be eventually moving out of using buefy?

@roiLeo
Copy link
Contributor

roiLeo commented Jan 10, 2023

@roiLeo also, since we no use oruga, will we be eventually moving out of using buefy?

FPRVpzPXIBwon9d

sorry I can't find the related issue on Nuxt3/Oruga-ui migration

@prachi00
Copy link
Member Author

prachi00 commented Jan 12, 2023

@roiLeo @yangwao I updated this PR with changes to links and have added colors I thought were right to themes.scss , I'll update it once @exezbcz provides the color documentation as well
In the meanwhile, can we merge it and make sure that people start following this approach from the next PRs and do not add anything in light.scss and dark.scss files as I will be removing it and I dont want to get conflicts later on?

@Jarsen136
Copy link
Contributor

Jarsen136 commented Jan 12, 2023

I really agree to find some smart way to make the theme colour switch more smoothly. IMO, we could implement the theme switch by using CSS variables. (ref: https://stackoverflow.com/questions/64390664/how-to-implement-switchable-themes-in-scss)

For example, in the nft-gallery project. We could define different colors for one variable on light.css and dark.css files. On the component side, the variable could be used as var(xxxx) to transform to the real color value.

styles/themes/_light.scss

html.light-mode {
 --background-color: white;
 --title-color: #333;
}

styles/themes/_dark.scss

html.dark-mode {
 --background-color: black;
 --title-color: #fff;

}

component css file

.card {
  color: var(--title-color);
  background: var(--background-color);
}

IMO, CSS variables would be easier to use and also be more readable in the codebase. WDYT @roiLeo @prachi00

@roiLeo
Copy link
Contributor

roiLeo commented Jan 12, 2023

IMO, CSS variables would be easier to use and also be more readable in the codebase. WDYT @roiLeo @prachi00

Sure should work too, we have already implemented something like this in libs/ui

.dark-mode {
--card-box-shadow: 4px 4px white;
}
.light-mode {
--card-box-shadow: 4px 4px hsl(0deg, 0%, 4%);
}

but I remember posting a comment with @preschian because I was stuck on a case where I had to override variable

Could you open a small PR with your poc? maybe start by tweaking colors on NeoSelect component

edit: I think it was because I was trying to set sass variable with css variable or vice versa

@Jarsen136
Copy link
Contributor

Jarsen136 commented Jan 12, 2023

IMO, CSS variables would be easier to use and also be more readable in the codebase. WDYT @roiLeo @prachi00

Sure should work too, we have already implemented something like this in libs/ui

.dark-mode {
--card-box-shadow: 4px 4px white;
}
.light-mode {
--card-box-shadow: 4px 4px hsl(0deg, 0%, 4%);
}

Yes, this part using CSS variables is what I have implemented in https://github.com/kodadot/nft-gallery/pull/4549/files.
It could also be my POC of using CSS variables : )

but I remember posting a comment with @preschian because I was stuck on a case where I had to override variable
Could you open a small PR with your poc? maybe start by tweaking colors on NeoSelect component

edit: I think it was because I was trying to set sass variable with css variable or vice versa

There is nothing special to override some properties of css with css variables.
For example, in this case

var(--card-box-shadow) would be auto converted to white colour in dark mode.

.card {
    box-shadow: var(--card-box-shadow);
}

If we would like to override it, we could do something like this. Or simply replace the var(xxx) with anything you want.
Note that do not try to change the value of the variables in the specific component.

.dark-mode {
  .card {
    box-shadow: 2px 2px #333;
  }
}

but I remember posting a comment with @preschian because I was stuck on a case where I had to override variable

If the problem is still there, I would like to take a look.

@roiLeo
Copy link
Contributor

roiLeo commented Jan 12, 2023

There is nothing special to override some properties of css with css variables. For example, in this case

var(--card-box-shadow) would be auto converted to white colour in dark mode.

.card {
    box-shadow: var(--card-box-shadow);
}

How will you handle/override default bulma var? (in /styles/abstracts/derived-variables)

$primary: var(--primary); doesn't look like working

@Jarsen136
Copy link
Contributor

How will you handle/override default bulma var? (in /styles/abstracts/derived-variables)

$primary: var(--primary); doesn't look like working

To my knowledge, bulma var could only be overridden to a specific value. I guess we should not try to convert it into different colors accordingly. If a component using color: $primary, we could use color: var(--primary) !important to override it. Could it solve the case you meet?

@roiLeo
Copy link
Contributor

roiLeo commented Jan 12, 2023

To my knowledge, bulma var could only be overridden to a specific value. I guess we should not try to convert it into different colors accordingly. If a component using color: $primary, we could use color: var(--primary) !important to override it. Could it solve the case you meet?

I don't think so we'll need to rewrite every scss component file that use $primary variable.
Take a look at https://github.com/jgthms/bulma/tree/master/sass we still can override scss variables but it won't work with css variable :/

@Jarsen136
Copy link
Contributor

To my knowledge, bulma var could only be overridden to a specific value. I guess we should not try to convert it into different colors accordingly. If a component using color: $primary, we could use color: var(--primary) !important to override it. Could it solve the case you meet?

I don't think so we'll need to rewrite every scss component file that use $primary variable. Take a look at https://github.com/jgthms/bulma/tree/master/sass we still can override scss variables but it won't work with css variable :/

You're right. With this approach, default scss variables could be overridden properly.

@prachi00
Copy link
Member Author

soooo which approach are we following?

@roiLeo
Copy link
Contributor

roiLeo commented Jan 16, 2023

soooo which approach are we following?

Yours I think since we won't be able to override default bulma vars with custom css

@prachi00
Copy link
Member Author

soooo which approach are we following?

Yours I think since we won't be able to override default bulma vars with custom css

so can we merge this and make sure people follow this in the future prs and not add anything to dark and light scss files?

@preschian
Copy link
Member

@prachi00 maybe set this PR to ready for review, so the others can start reviewing
image

@prachi00 prachi00 marked this pull request as ready for review January 18, 2023 18:09
@prachi00 prachi00 requested a review from a team as a code owner January 18, 2023 18:09
@prachi00 prachi00 requested review from roiLeo and Jarsen136 and removed request for a team January 18, 2023 18:09
@prachi00
Copy link
Member Author

@preschian @roiLeo @Jarsen136 can we please merge this so I can continue with this further
Also, @exezbcz can you confirm the colours on figma please for dark and light?

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks strange to use, but if it's ok for @kodadot/code-review-guild I'm fine with that.

I see that there are still a lot of changes, I would have liked to try to overload the bulma variables first, fingers crossed that nothing breaks 🤞

@Jarsen136
Copy link
Contributor

It looks strange to use, but if it's ok for @kodadot/code-review-guild I'm fine with that.

I see that there are still a lot of changes, I would have liked to try to overload the bulma variables first, fingers crossed that nothing breaks 🤞

The usage of mixin css looks strange also to me. I would prefer to use CSS variables to override custom colors(k-dark\k-grey), which would be easy to use and read. The mixin method is also necessary to override the oruga variables(like $primary).

For now, let's merge it until we find a more property approach to handle it.

@exezbcz
Copy link
Member

exezbcz commented Jan 20, 2023

@preschian @roiLeo @Jarsen136 can we please merge this so I can continue with this further Also, @exezbcz can you confirm the colours on figma please for dark and light?

They are mostly the same

The tricky ones

accentlight - for button hover in light mode
accentlight2 - for hover over bigger areas, like search results (light mode)

dark mode alternatives:

  • accentlight2 is accentlight2dark
  • accentlight hover its k-accent and text to black color

Others

There are not many other variants, then there are few grey colors:

k-grey, usually used for dividers, second priority text.
k-shade, for innactive states:
image
divider (somewhere)
image
and used when k-grey is to grey :D

hover on text:
k-hovergrey - from black text link
dark mode its k-shade (from white text to k-shade color on hover)

k-darkmode:

  • used everywhere in dark mode instead of black

k-black

  • black color for light mode, in dark mode its k-darkmode

Then there is a single type of blue and a lighter variation that is only rarely used.

those are the main colors, its only few of them because i did not really need much more. Once i finish redesigning, i will try to put it together somehow and make a better design system than this. Now the naming sucks a bit - i know it and i am sorry for that. Fortunately we have option to see which color was used in figma, you can check that as well if you dont.

All the colors can be seen in landing page handoff first page. I usually update it when i introduce new color. You can refer to that if something is unclear.

Thanks!

figma link: https://www.figma.com/file/3iOjW12ERFRDSVnpLuhQmf/landing-page-handoff?node-id=1305%3A5612&t=WDzZSpIFVo1HZQNu-1

@exezbcz
Copy link
Member

exezbcz commented Jan 20, 2023

if i understand this correctly, changing the hex codes and the names of the colors should be easier now?

@prachi00
Copy link
Member Author

if i understand this correctly, changing the hex codes and the names of the colors should be easier now?

yes it should be

@codeclimate
Copy link

codeclimate bot commented Jan 21, 2023

Code Climate has analyzed commit 6b18442 and detected 0 issues on this pull request.

View more on Code Climate.

@prachi00
Copy link
Member Author

yeah I'll be making the changes after merging it to avoid conflicts and we have to make sure that all the theming issues follow this approach now

@prachi00 prachi00 merged commit 1ef567d into main Jan 21, 2023
@prachi00 prachi00 deleted the feat-theming branch January 21, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS Variables for theming
6 participants